Skip to content

fix(formatter): account lambda beginning when formatting method call#7857

Merged
asterite merged 4 commits intomasterfrom
ab/formatter-bug-lambda
Mar 31, 2025
Merged

fix(formatter): account lambda beginning when formatting method call#7857
asterite merged 4 commits intomasterfrom
ab/formatter-bug-lambda

Conversation

@asterite
Copy link
Copy Markdown
Collaborator

@asterite asterite commented Mar 31, 2025

Description

Problem

Resolves #7407

Summary

The formatter is pretty complex when it comes to deciding how to format method calls, call chains and lambdas but I think the fix here is the "correct" one. The explanation for this change is a bit on line 642-664 of chunks.rs, then on 674-680.

The code in the issue:

comptime fn generate_compute_note_hash_and_optionally_a_nullifier() -> Quoted {
    let notes = NOTES.entries();
    if notes.len() > 0 {
        for i in 0..notes.len() {
            let (typ, (_, _, _, _)) = notes[i];

            let get_note_type_id = typ.get_trait_impl(NOTE_INTERFACE).unwrap().methods().filter(|m| {
                m.name() == quote { get_note_type_id }
            })[0]
                .as_typed_expr();
        }

        quote {}
    } else {
        quote {}
    }
}

now gets formatted like this:

comptime fn generate_compute_note_hash_and_optionally_a_nullifier() -> Quoted {
    let notes = NOTES.entries();
    if notes.len() > 0 {
        for i in 0..notes.len() {
            let (typ, (_, _, _, _)) = notes[i];

            let get_note_type_id = typ
                .get_trait_impl(NOTE_INTERFACE)
                .unwrap()
                .methods()
                .filter(|m| m.name() == quote { get_note_type_id })[0]
                .as_typed_expr();
        }

        quote {}
    } else {
        quote {}
    }
}

Note how the method call was split into multiple lines. Before this change it would check what's the width of this line:

            let get_note_type_id = typ.get_trait_impl(NOTE_INTERFACE).unwrap().methods().filter(

It's 96, so less than 100, so it would put all of that on one line then proceed to format the lambda. However, there's still "|m| {" in that line... it could be moved to a separate line but it would look strange. So, this PR is changed to take that piece of code into account when determining whether that first part of the call fits into a single line. In this case it doesn't anymore, so the call chain is split into multiple lines.

rustfmt formats the code in the same way (maybe they use a similar logic).

Additional Context

I also added a couple of Display implementations because they are useful when debugging these things.

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite marked this pull request as ready for review March 31, 2025 15:17
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 10a4cd9 Previous: 053ac2b Ratio
olehmisar_noir_webauthn_ 2 s 1 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite asterite requested a review from a team March 31, 2025 15:29
@asterite asterite added this pull request to the merge queue Mar 31, 2025
Merged via the queue into master with commit 9dc5818 Mar 31, 2025
115 checks passed
@asterite asterite deleted the ab/formatter-bug-lambda branch March 31, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Running nargo fmt repeatedly results in cycle of self-reverting changes

2 participants